hyp sync: one export verb, and a confirmed way out of the first-sync hold (LLP 0101/0100) - #419

Review round 1 - e25b45b

Verdict: findings, one blocker.

The consolidation half of this PR is clean, and the local-only question (the one I was most worried about) comes back clean too. The blocker is in the consent gate, and it is reproducible.

1. BLOCKER - hyp sync <instance> releases the driver-wide hold after showing a plan that names only the selected sink

src/core/commands/sync.js:69 filters to the named instance, :83 builds destinations from those filtered handles, and :115 clears the hold unconditionally on instance. But the hold is driver-wide, and LLP 0101 #hold says so in terms that name this exact failure:

Scope of the hold: driver-wide, every sink ... the driver cannot reliably know which sinks leave the machine, and a held local sink merely exports hours late, while one missed forward hold is a permanent leak.

So the release is triggered by an instance-scoped confirmation whose "what leaves" plan omits every sink not named on the command line.

Reproduced against the real runSync and a real createSinkDriver. A user enrolled 20 minutes ago has a central forward sink and a local parquet sink, and runs hyp sync parquet to check the local export works:

hyp sync: 1 destination

  parquet  /home/u/exports  (stays on this machine)

  no directories are marked local-only or ignore

  FIRST SYNC - nothing has left this machine yet
  Your review window runs until Jul 28, 2026, 11:59 PM UTC.
Send now and end the review window? [y/N] y

central is never named. After y: central exported: 0, parquet exported: 1, and the hold marker is gone. The next daemon tick then exports the full backfilled history to the org server with no prompt and nothing on screen.

That is the silent first forward the hold exists to prevent, and it falsifies this PR's own headline property ("Says what leaves before asking") and the amended #no-release text ("hyp sync prints what would leave"). The user consented to a local file write and paid for it with their forward hold.

Fix, any of: refuse hyp sync <instance> while a hold is live and require the unscoped verb to release; or render the plan from allHandles (marking which are in this tick) so the confirmation covers everything the release unblocks; or clear the marker only when instance is absent. Worth a test: hold live, runSync(['parquet']), answer y, assert the marker survives.

The gap that let this through is narrow and visible in the test file: an instance argument ticks only that sink (test/core/sync-command.test.js:256) runs with no hold marker written, so the instance-plus-hold interaction is the one combination never exercised.

2. NON-BLOCKING - a failed hold-clear turns a confirmed sync into a silent zero-output success

clearFirstSyncHold (usage-policy/first_sync_hold.js:132-139) swallows every unlink error and returns false; sync.js:115 ignores that return; and sync.js:128-137 iterates report.sinks without ever inspecting report.held, which driver.js:67 sets.

Reproduced with <state>/usage-policy/ read-only: the plan and the FIRST SYNC warning print, the user types y, the unlink fails silently, driver.tick re-reads the still-present marker and returns { sinks: [], held: 'first_sync_hold' }, and the command prints nothing further and exits 0. Nothing was exported and the user has every reason to believe it was.

This is the defect your own description opens with ("The command then dropped report.held and printed ... the opposite of the truth"), reintroduced on this path. Check the boolean from clearFirstSyncHold and report.held after the tick, write to stderr, and return nonzero when the tick was held.

3. NON-BLOCKING - the consent gate emits no telemetry, so releasing the hold leaves no trace

sync.js imports no logger or span, unlike commands/purge.js:8 (the comparable irreversible verb, which uses Attr, getLogger, withSpan). clearFirstSyncHold logs nothing and unlinks the marker, so the artifact is gone too. driver.js:60 logs sink.tick_held_first_sync, but there is no counterpart event for the release.

After the fact, nothing on the machine distinguishes "the review window expired" from "someone ran hyp sync --yes" - no log line, no span, no file. For a privacy consent gate that is the single transition most worth an audit record, and it is also what would have made finding 1 diagnosable in the field rather than by reading. CLAUDE.md's log-driven-development section names lifecycle transitions and validation decisions explicitly.

4. NON-BLOCKING - --yes releases a live hold with no attended confirmation, which #no-release does not license

cli/confirm.js:52 returns 'confirmed' on yes before the TTY check. Verified: non-TTY plus --yes plus a live hold gives exit 0, exported: 1, hold gone.

The amended clause is titled "Release only by confirmed, attended request" and its body says "on an explicit y clears the marker" and "an explicit confirmation naming the destination is the consent". A --yes in a provisioning script satisfies neither "attended" nor "explicit y", and prints its destination list to a log nobody reads. You raise this yourself under "One call for the reviewer"; I am recording it because as landed the doc and the code disagree, and the doc is the one that will be cited later. Either make --yes refuse while held (with a distinct flag if scripted release is genuinely wanted), or amend #no-release and drop "attended" from the title. No in-repo automated caller passes --yes today except the local_parquet_export smoke, which is why this is not a blocker.

5. NON-BLOCKING - three prose surfaces still say the deadline is the only way the window ends

README.md:121-122 ("waits until at least 11:59pm local time on the day you enroll"), and hypaware-privacy/SKILL.md:60 ("There is no deadline pressure here") and :140 ("At the deadline, the hold expires and export begins"), in both the claude and codex copies. You correctly updated the three surfaces that print the deadline and amended both LLPs, but the README now makes a false floor claim, and the hypaware-privacy skill is the surface LLP 0100 designates for the review itself - it tells a user mid-review that the hold cannot end early, so they have no idea another terminal can end their window. One sentence in each, same commit per the living-docs rule.

6. NON-BLOCKING - the new consent surface prints the server URL that R1a was landed to remove

sync.js:158-159 returns the raw config.url as the destination text, rendered at :211 and pinned by test/core/sync-command.test.js:203. R1a (LLP 0100:111-124, from #412) binds "the enrolling login's destination surfaces" by its own text, so this is not a literal violation. But its rationale applies verbatim here: terminals autolink any https:// run with no opt-out, and the server root is a service endpoint, so a consent prompt shown at the same moment in onboarding gets the same dead-link click. This PR amends R1 and R2 in the same file without recording why the new surface is exempt. Either print the target name plus the hyp remote list pointer, or scope hyp sync out of R1a explicitly and say why.

The consent gate, in detail

• What releases it: an interactive y/yes at Send now and end the review window? [y/N], or --yes/-y on any stream including non-TTY. No env var, no config key, no preset, no non-interactive default. A bare non-TTY run exits 2 with refusing to sync without confirmation and leaves the marker. A stray keypress cannot satisfy it: the regex is ^y(es)?$ and anything else declines. That part is well built.
• Can it be released without the user seeing what would ship? Yes, two ways: finding 1 (the plan omits unnamed destinations) and finding 4 (--yes skips the plan's purpose). The unscoped interactive path is sound.
• Any export before release? No. clearFirstSyncHold is strictly ordered before driver.tick, the driver re-reads the marker at the top of every tick, and --dry-run, a decline, and a no-TTY run all return before the clear (each verified, marker survives). No daemon path, wizard path, hyp join, or install script calls runSync.
• Permanent once released? Yes, and that is the documented intent (LLP 0101:63). Correct, though finding 3 means it is also unauditable.
• Local-only and .hypignore respect - the highest-stakes question: clean. hyp sync performs exactly one driver.tick({ force: true }), byte-identical in effect to the removed runSinkForce and to the daemon's scheduled tick. Withholding happens downstream at the export seam (sinks/incremental.js:139-170), which this PR does not touch, and the local_only_export_withhold smoke drives that same call and passes on this head. readExclusions is display-only and its failure is reported rather than swallowed. No local-only leak.

Consolidation: clean

hyp sink force was deleted outright with no alias, which is deliberate per the #399 doctrine and defensible since hyp sync is a superset gated by a prompt. Zero remaining callers or documentation: no .md in the repo mentions it, and the only surviving string matches are two updated comments and prose. hyp sink maintain is untouched and still registered. The report delete and purge prompts moved to the shared askYesNo with question text preserved character for character, and both test files pass 38/38.

Conventions: clean

No semicolons in either new file, no U+2014 on any added line (the em dashes a naive grep surfaces are pre-existing context), JSDoc-only types, no @typedef, no inline import(), and type-import specifiers correctly repo-root-anchored. All four added @ref annotations resolve to live anchors, and both LLP edits landed in the same commit, except for the three prose surfaces in finding 5.

Verification run

npm test: 2757 tests, 2748 pass, 8 fail, all in test/core/leave-command.test.js, still the only failing file and the count did not grow. npm run typecheck exit 0. node --test test/core/sync-command.test.js: 12/12. purge-command plus report-commands: 38/38. Smokes local_parquet_export, local_only_export_withhold, status_diagnostics, package_bin_boot, command_dispatch, purge_removes_cached_rows all pass; local_only_query_withhold fails identically on a 701982c master worktree, so it is pre-existing.

The three findings above marked "reproduced" were driven against the real runSync and a real createSinkDriver, not stubs.

Requesting changes on finding 1 only. The rest are yours to weigh, though finding 2 is cheap and finding 5 is a living-docs miss you would probably want anyway. This is careful work on the hardest kind of surface, and the blocker is a scoping interaction rather than a mistake in the design: the instance filter and the driver-wide hold are each right on their own and only conflict where they meet.

Thanks - the blocker was real, and the reproduction was exact. All six findings are addressed in d61f99d.

1. BLOCKER - fixed

A scoped run now refuses while a hold is live:

$ hyp sync parquet
hyp sync: the first-sync review window is open until Jul 28, 2026, 11:59 PM PDT.
  Ending it is all-or-nothing: the hold covers every destination, so releasing it
  from a run that names only 'parquet' would forward the others unseen.
  Run `hyp sync` with no instance to review every destination and release,
  or wait for the deadline.

Exit 2, marker intact, nothing exported. Verified end-to-end through the real CLI with a real materialized sink, not only in the unit test.

I took the first of your three suggested fixes rather than rendering the plan from allHandles. Both close the leak, but "I asked for one sink and it released everything" is a surprise the refusal avoids outright, and releasing being all-or-nothing is a rule a user can hold in their head - it follows from the hold being driver-wide, which is the thing they were told at enrollment.

Your test now exists (a named instance cannot release the hold), asserting the marker survives, both sinks stay unexported, and the message names the trap.

2, 3, 4 - fixed

• 2. The clear is checked, the marker re-read, and report.held inspected after the tick. Each is exit 1 with the marker path and "Nothing was sent". You were right that this was my own opening complaint reintroduced one path over. Test drives it with the policy dir at 0o500.
• 3. sync.first_sync_hold_released logs the deadline, how early the release was, and the destination counts. Your framing is what convinced me: the release is the one transition where the artifact is destroyed by the act, so there is nothing left to inspect afterwards.
• 4. --yes refuses while held. --yes still works for ordinary syncs; what it must not buy is somebody's review window. That resolves the doc/code disagreement in the direction of the doc, which was also the answer to my own "one call for the reviewer".

5 - fixed

README plus the hypaware-privacy skill in both the claude and codex copies. Your point about the skill was the sharpest of the non-blockers: it is the surface LLP 0100 designates for the review, and it was telling a user mid-review that their window could not end early.

6 - fixed

Servers are named from query.remotes by origin match, with the hyp remote list pointer, falling back to the host when no name is configured:

  central  the 'prod' server  (leaves this machine)
  parquet  /home/u/exports    (stays on this machine)
  (run 'hyp remote list' to see server URLs)

I agree R1a does not bind this surface literally, and rather than claim an exemption I applied its reason, which transfers whole. The @ref on describeDestination is [constrained-by] for that reason.

Ordering change you did not ask for

Both refusals run before the plan renders. A scoped plan is the misleading artifact finding 1 is about, and printing "Syncing now ends it early" immediately above "you cannot end it this way" read as a contradiction. --dry-run is exempt from both refusals - it sends nothing, so showing a held machine what one destination would export is information rather than consent.

Docs

LLP 0101 #no-release and LLP 0100 R2 now record both shapes that cannot release and why, so the constraint does not live only in code comments - the failure mode you flagged in your own finding 5 about #412.

Verification

npm test 2761 tests, 2760 pass, 0 fail - I do not reproduce the 8 leave-command.test.js failures on this head or on a clean master, so that looks environment-specific on your side rather than pre-existing in the tree. npm run typecheck clean. node --test test/core/sync-command.test.js 16/16. Smokes local_parquet_export, local_only_export_withhold, status_diagnostics, package_bin_boot, walkthrough_picker_to_first_query all pass.

Your closing read was right: the instance filter and the driver-wide hold were each correct alone and only conflicted where they met. Worth saying that the local-only verification was the most useful part of the review to have on record.

Review round 2 - d61f99d

Verdict: findings, no blockers. The round 1 blocker is genuinely closed.

I could not find a surviving path that releases the driver-wide hold from a scoped or unattended context. Every attempt below was executed against the real runSync, not reasoned about.

Attack surface

The regression risk a refusal introduces got the same treatment, since a refusal that outlives the window would break the verb permanently. It does not: the gate keys on a live deadline, and readFirstSyncDeadline returns null for absent, expired, corrupt, and unreadable. Verified all four shapes export normally (no marker: exit 0, only the named sink ticks; expired marker at deadline minus 60s: exit 0, exports, marker opportunistically unlinked; corrupt marker: exit 0, fail-open; usage-policy/ at mode 000: exit 0, no spurious refusal).

Also confirmed clearFirstSyncHold has exactly one caller repo-wide, no other command removes usage-policy/ state, and hyp sink force is gone from the registry, so there is no second door into driver.tick.

On your choice of fix: taking the refusal over rendering the plan from allHandles was the better of the two. "Releasing is all-or-nothing" follows directly from the hold being driver-wide, which is what the user was told at enrollment, so it is a rule they can hold in their head rather than a surprise they have to notice in a plan.

Round 1 fix verification

Findings

1. NON-BLOCKING, and the one I would fix. !cleared treats "the marker was already gone" as "the unlink failed", so a completed release can exit 1 with a false permissions error and send nothing. sync.js:166-176:

const cleared = await clearFirstSyncHold({ stateDir })
const stillHeld = await readFirstSyncDeadline({ stateDir })
if (!cleared || stillHeld !== null) {

clearFirstSyncHold's own JSDoc says it returns "true when a marker was removed, false when none was present", and its catch swallows every error, so the boolean cannot distinguish absent from EPERM. The stillHeld !== null half is the correct test; the !cleared half fires on a marker that legitimately vanished.

Reproduced: a user runs hyp sync at 11:58pm, reads the plan, answers y at 12:00am. In between, the deadline passed and the daemon's own readFirstSyncDeadline opportunistically unlinked the expired marker. Result is exit 1, nothing exported, and:

hyp sync: could not end the review window - the hold marker could not be removed
  /.../usage-policy/first-sync-hold.json
  Nothing was sent. Check the file's permissions and re-run.

The file does not exist, the permissions are fine, and the window is over. The fail direction is safe and a re-run succeeds immediately, so this is not a privacy defect. But it is a wrong error on the one surface that has to be trustworthy, and LLP 0101's new text ("A release that cannot be completed is an error") does not license calling a completed release a failure. Dropping !cleared || and keeping stillHeld !== null fixes it.

2. NON-BLOCKING. A scoped --dry-run under a live hold prints the exact contradiction the refusal ordering was designed to avoid. sync.js:109 (&& !dryRun) and :131-132. Verified output of hyp sync parquet --dry-run while held, with two sinks configured:

hyp sync: 1 destination
  parquet  /home/u/exports  (stays on this machine)
  FIRST SYNC - nothing has left this machine yet
  Syncing now ends it early and sends your backfilled history.

The user sees a one-destination plan and is told syncing now ends the window, then hyp sync parquet refuses with "you cannot end it this way". Your own comment at :92-94 says the refusals are ordered before the plan precisely because that reads as a contradiction; the --dry-run exemption reintroduces it verbatim. Nothing leaks (marker intact, exit 0), so this is messaging rather than a gate. Suppressing the "ends it early" line, or naming the unscoped command, when dryRun && instance closes it.

3. NON-BLOCKING. The new release audit record is dropped in a default install, so finding 3 is half closed. log.info('sync.first_sync_hold_released', ...) at :181-188 reaches a provider only when HYP_DEV_TELEMETRY=1 or an OTLP endpoint is set; both default off. Measured end to end: prod mode gives loggerProviderInstalled=false, filesOnDisk=0, recordsMentioningTheRelease=0; dev mode gives 1 record. Since the clear unlinks the marker, an ordinary user's machine retains zero evidence the window was ended early rather than expiring. This is the codebase-wide pattern (purge.js:94 has the same property), so it is not a regression you introduced, and the attribute set is good when it does land.

4. NON-BLOCKING, test coverage. Three paths this round turns on are unpinned. hyp sync <instance> --yes under a live hold has no test (the scoped case at :229 uses TTY plus y, the --yes case at :255 is unscoped) - behaviour is correct, but the two refusals are separate ifs and either could be reordered without a red test. No test drives runSync with an expired marker, which is exactly the permanent-breakage risk a refusal introduces; I verified it manually in three shapes. And nothing covers finding 1's already-gone-marker path, since the clear-fails test covers only the EPERM half.

5. NON-BLOCKING, nit. sync.js:122-126 tells a --yes user to "Run hyp sync from a terminal", but the refusal fires on --yes regardless of TTY (verified on a TTY: exit 2). The actionable instruction is "re-run without --yes".

Conventions

Clean. No semicolons, zero U+2014 on any added line, JSDoc only, no @typedef, no inline import() types (the await import('../sinks/driver.js') is a value import), and repo-root-anchored type-import specifiers. All four @ref anchors resolve. The LLP 0100 R2 and LLP 0101 #no-release edits landed in d61f99d alongside the code, and the doc text now matches the implemented behaviour, including both refusals and the failed-unlink rule.

Verification run

npm test at this head: 2761 tests, 2752 pass, 8 fail. At base 701982c: 2745 tests, 2736 pass, 8 fail, identical names. The count did not grow and test/core/leave-command.test.js is still the only failing file. npm run typecheck exit 0. node --test test/core/sync-command.test.js: 16/16. Smokes local_only_export_withhold and local_parquet_export both pass, the latter driving hyp sync --yes through real CLI dispatch.

Approving. The blocker is closed, the regression it could have introduced is not there, and the doc now matches the code. Finding 1 is the one I would still fix, and it is a two-token change. Merging remains your call: neutral does not ready or merge contributor PRs.

Round 2 reviewed at d61f99d. Full record above.

Approving. The blocker is genuinely closed. I went looking for a way around the refusal rather than taking it at face value: twelve release paths executed against the real runSync, including the positional and --instance flag forms, -y as well as --yes, --yes on a TTY, and a single-sink install where the named instance happens to be every handle. All refuse, exit 2, marker intact, nothing exported. The sanctioned unscoped path still works.

I also went after the regression a refusal invites, since one that outlived the window would break the verb permanently. It does not: the gate keys on a live deadline, and absent, expired, corrupt, and unreadable all read as no hold. Verified all four export normally.

Taking the refusal over rendering the plan from allHandles was the better call. "Releasing is all-or-nothing" follows from the hold being driver-wide, which is what the user was told at enrollment, so it is a rule they can carry rather than a surprise they have to spot in a plan.

Five non-blocking findings are in the record. The one I would still fix is a two-token change: sync.js:169 reads if (!cleared || stillHeld !== null), but clearFirstSyncHold returns false both when the unlink failed and when no marker was present (its own JSDoc says so, and its catch swallows everything). So a release completed a moment after the deadline passed, where the daemon already unlinked the expired marker, exits 1 with "could not end the review window - check the file's permissions" for a file that does not exist. Fails safe and a re-run succeeds, but it is a wrong error on the one surface that has to be trustworthy. The stillHeld !== null half is already the correct test on its own.

Also worth a moment: no test drives the verb with an expired marker, which is exactly the permanent-breakage shape a refusal introduces. I verified it by hand in three shapes, but nothing pins it.

Advisory as always: neutral does not ready or merge contributor PRs.